Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature / Columnar data C API #711

Merged
merged 23 commits into from
Sep 17, 2024
Merged

Conversation

Jerry-Jinfeng-Guo
Copy link
Contributor

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo commented Sep 3, 2024

Implements C API changes in #548

  • interface
  • add to c++ api wraper
  • c api tests
    • api model
    • serialization
  • fix ... things that inevitably need fixing

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo added the feature New feature or request label Sep 3, 2024
@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo self-assigned this Sep 3, 2024
Signed-off-by: Jerry Guo <[email protected]>
…aset. `main_model_impl` still lacks functionality to accommodate columnar data input

Signed-off-by: Jerry Guo <[email protected]>
@mgovers mgovers changed the base branch from main to feature/main-model-columnar-data September 9, 2024 14:31
@mgovers mgovers force-pushed the feature/columnar-data-c-api branch from 2001844 to 874c770 Compare September 10, 2024 07:09
Signed-off-by: Martijn Govers <[email protected]>
Copy link
Member

@mgovers mgovers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great cleanup on the clang-tidy

tests/c_api_tests/test_cpp_api_model.cpp Outdated Show resolved Hide resolved
Signed-off-by: Martijn Govers <[email protected]>
Signed-off-by: Martijn Govers <[email protected]>
Base automatically changed from feature/main-model-columnar-data to main September 10, 2024 09:59
An error occurred while trying to automatically change base from feature/main-model-columnar-data to main September 10, 2024 09:59
@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo marked this pull request as ready for review September 13, 2024 14:44
Copy link
Contributor

@figueroa1395 figueroa1395 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me overall, just minor comments and the possible addition of tests for add_attribute_buffer for deserialized data, may be needed.

Copy link
Member

@TonyXiang8787 TonyXiang8787 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Jerry-Jinfeng-Guo please resolve the merge conflict and we should be good to go

@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo marked this pull request as draft September 16, 2024 11:43
@Jerry-Jinfeng-Guo Jerry-Jinfeng-Guo marked this pull request as ready for review September 16, 2024 15:40
@TonyXiang8787
Copy link
Member

Hmmm. buffer overflow...

Signed-off-by: Jerry Guo <[email protected]>
@Jerry-Jinfeng-Guo
Copy link
Contributor Author

Hmmm. buffer overflow...

Fixed... |complete json data| < |json data|

Copy link

@TonyXiang8787 TonyXiang8787 added this pull request to the merge queue Sep 17, 2024
Merged via the queue into main with commit 5e199a4 Sep 17, 2024
26 checks passed
@TonyXiang8787 TonyXiang8787 deleted the feature/columnar-data-c-api branch September 17, 2024 07:27
@mgovers mgovers mentioned this pull request Nov 5, 2024
27 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants